1301138 - Clicking on a Giphly embed in Twitter loads about:blank

Status: VERIFIED FIXED

(Core :: DOM: Security, defect)

Description

[Kevin Brosnan [Ex-Mozilla]]

* Open https://twitter.com/mmmandel/status/773528085803077632
* Click once to load the embed
* Click again on the image (not the text below the image)

Expected results: New tab with giphly page loaded

Actual results: about:blank

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=49f41e6097076c35c92082cb26
8ea63b9ea5ab03&tochange=58d84f830e72fece44a4838ca47781a3b754aea1

bz wrote all the patches in the regression range, ni to comment. Tentatively blocking on bug 1190641

Comment 1

[Kevin Brosnan [Ex-Mozilla]]

Fixed link https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=49f41e6097076c35c92082cb268ea63b9ea5ab03&tochange=58d84f830e72fece44a4838ca47781a3b754aea1

Comment 2

[Boris Zbarsky [:bzbarsky]]

Simple testcase:

  <iframe srcdoc="<a href='http://mozilla.org' target='_blank'>Link to mozilla.org</a>"
          sandbox="allow-popups allow-popups-to-escape-sandbox allow-same-origin
                   allow-scripts">
  </iframe>

The allow-popups-to-escape-sandbox bit is key.  Without that things work.  Looking into why this is failing, but chances are it's because the spec is broken and targeted anchors are not affected by bug 960545.  See also bug 1190641 comment 20 and <https://github.com/whatwg/html/issues/1218>....

I strongly believe we should fix this before shipping 49 (e.g. by disabling bug 1190641 there; still looking into what the simplest possible fix is).

Comment 3

[Boris Zbarsky [:bzbarsky]]

Based on testing Chrome, it looks to me like they simply disable the check for the one permitted sandbox navigator when "allow-popups-to-escape-sandbox" is set or something.  For example, this testcase:

  <a href="http://example.com" target="foo">Click me first</a>
  <iframe srcdoc="<a href='http://mozilla.org' target='foo'>Click me second. Did I navigate?</a>"
          sandbox="allow-popups-to-escape-sandbox">
  </iframe>

performs the navigation.  But switching the target in the iframe to '_top' blocks the navigation in Chrome...

Looking into their source now to see if I can figure out exactly what they do.

Comment 4

[Liz Henry (:lizzard) (relman/hg->git project)]

We may need to block 49 on this issue. bz will comment further...

Comment 5

[Boris Zbarsky [:bzbarsky]]

OK.  So I checked the Chrome source and I was right.  They simply don't enforce https://html.spec.whatwg.org/multipage/browsers.html#allowed-to-navigate step 3 if A doesn't have the "sandbox propagates to auxiliary browsing contexts" flag.

At this point, I think we have four options for Firefox 49:

1) Implement the Chrome behavior.  This, I believe, opens up a hole
   in sandboxing; the testcase in comment 3 shows the sandboxed page
   navigating a page that the parent opened...
2) Ship our existing behavior.  This breaks pages, clearly.
3) Unship support for allow-popups-to-escape-sandbox.  This puts us at no worse
   a place than 48, but this feature is somewhat widely anticipated because it 
   allows ads to be sandboxed that otherwise can't be sandboxed at all.  In
   terms of other browser support, Safari (shipping; webkit nightly is different)
   doesn't apply the sandbox to popups at all.
4) Ship some other fix (e.g. setting the one permitted sandboxed navigator
   even though we're escaping the sandbox).

#1 is probably most web-compatible, but as I said I believe it introduces a security hole and poisons the well in terms of being able to remove that security hole, since both Chrome and we would be shipping it.

#2 is not something I really want to do, honestly.  Especially because I expect people to use this sandbox flag more and more, _especially_ once we announce we're shipping it in 49.  I expect we would see a ton of breakage if we did this.

#3 is probably safest in some sense, but runs the risk of some pages being broken anyway, if they use the flag to get the behavior they want in Chrome and get it automatically in Safari.  This is a pretty theoretical breakage, though; we have no concrete examples yet.

#4 runs the risk of us shipping something that still leads to _some_ pages somewhere being broken and then having to change the behavior again if/when the spec actually gets sorted out to not suck.  Of course it's possible that #4 _is_ what will end up getting standardized.  It seems like a sane solution to me, but I haven't dug through all the edge cases here.

Olli, do you have any thoughts on those options?  Bob, likewise?

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: A viable option 4 implementation. Fixes the testcase, at least

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8789075 [details] [diff] [review]
A viable option 4 implementation.  Fixes the testcase, at least

>@@ -1048,26 +1048,27 @@ nsWindowWatcher::OpenWindowInternal(mozI
>   // better have a window to use by this point
>   if (!newDocShellItem) {
>     return rv;
>   }
> 
>   nsCOMPtr<nsIDocShell> newDocShell(do_QueryInterface(newDocShellItem));
>   NS_ENSURE_TRUE(newDocShell, NS_ERROR_UNEXPECTED);
> 
>+  if (parentWindow) {
>+    newDocShell->SetOnePermittedSandboxedNavigator(
>+      parentWindow->GetDocShell());
>+  }

so we set OnePermittedSandboxedNavigator always, even if opener wasn't sandboxed at all. I guess that doesn't matter since the value is used only if opener is sandboxed.

Comment 8

[Boris Zbarsky [:bzbarsky]]

> so we set OnePermittedSandboxedNavigator always, even if opener wasn't sandboxed at all.

Oh, hmm.  That's not great, just on general "follow the spec" grounds.  How about I only set it if activeDocSandboxFlags is not 0?

Comment 9

[Boris Zbarsky [:bzbarsky]]

I'm going to commit this to inbound with the change from comment 8, so I can ask for branch approvals.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8789075 [details] [diff] [review]
A viable option 4 implementation.  Fixes the testcase, at least

Approval Request Comment
[Feature/regressing bug #]: Bug 1190641
[User impact if declined]: At least some links from twitter will be broken.
[Describe test coverage new/current, TreeHerder]: We now have web platform tests
   for this case.  We already had them for the window.open() cases.
[Risks and why]: See comment 5.  We're taking approach #4 from there.  I believe
   the risks are fairly low, but not entirely negligible.
[String/UUID change made/needed]: None.

Comment 11

[Carsten Book [:Tomcat]]

sorry boris had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=35521319&repo=mozilla-inbound

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/df7cb65bee39
Set the one allowed sandbox navigator even when popups are allowed to escape the sandbox, so the new popup can be navigated by the sandboxed document.  r=smaug

Comment 13

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/eeaebaf00e95
Backed out changeset df7cb65bee39 for wpt test failures

Comment 14

[Carsten Book [:Tomcat]]

note comment #12 is just after the backout information since there was a pulsebot problem, so the changeset is backed out and not relanded or so

Comment 15

[Bob Owen (:bobowen)]

(In reply to Boris Zbarsky [:bz] from comment #6)
> Created attachment 8789075 [details] [diff] [review]
> A viable option 4 implementation.  Fixes the testcase, at least

I think option 4, is the correct solution, given how most of the spec is written ... even though it's wrong on this one point.

I wish I'd thought through the implications further when I realised what I mentioned on irc (bug 1190641 comment 20) ... sorry.

I suppose one other option would be to allow the initial navigation, but not subsequent ones, in the sandbox escape case.
However, the genie is already out of the bottle, so I'm not sure that would make much difference and it would probably make the spec even more complicated.

Comment 16

[Bob Owen (:bobowen)]

(In reply to Carsten Book [:Tomcat] from comment #11)
> sorry boris had to back this out for test failures like
> https://treeherder.mozilla.org/logviewer.html#?job_id=35521319&repo=mozilla-
> inbound

Looks like this might just be missing the rename to helper-1 here:
https://hg.mozilla.org/integration/mozilla-inbound/file/df7cb65bee39/testing/web-platform/tests/html/semantics/embedded-content/the-iframe-element/iframe_sandbox_popups_nonescaping-1.html#l15

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Boris Zbarsky [:bz] from comment #8)
> Oh, hmm.  That's not great, just on general "follow the spec" grounds.  How
> about I only set it if activeDocSandboxFlags is not 0?
Sounds good.

Comment 18

[Bob Owen (:bobowen)]

(In reply to Bob Owen (:bobowen) from comment #16)
> (In reply to Carsten Book [:Tomcat] from comment #11)
> > sorry boris had to back this out for test failures like
> > https://treeherder.mozilla.org/logviewer.html#?job_id=35521319&repo=mozilla-
> > inbound
> 
> Looks like this might just be missing the rename to helper-1 here:
> https://hg.mozilla.org/integration/mozilla-inbound/file/df7cb65bee39/testing/
> web-platform/tests/html/semantics/embedded-content/the-iframe-element/
> iframe_sandbox_popups_nonescaping-1.html#l15

Confirmed this and as we want to get this landed ASAP, I'll push the amended patch.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f847d402cd2a
Set the one allowed sandbox navigator even when popups are allowed to escape the sandbox, so the new popup can be navigated by the sandboxed document.  r=smaug

Comment 20

[Marcia Knous [:marcia]]

Tracking 51+ for this regression.

Comment 21

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8789075 [details] [diff] [review]
A viable option 4 implementation.  Fixes the testcase, at least

Once this lands on m-c let's uplift to m-a, m-b, and m-r.

Comment 22

[Bob Owen (:bobowen)]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #21)
> Comment on attachment 8789075 [details] [diff] [review]
> A viable option 4 implementation.  Fixes the testcase, at least
> 
> Once this lands on m-c let's uplift to m-a, m-b, and m-r.

This will need a rebase ... same rebase for m-a/m-b I think and more to rebase for m-r.

It includes the web platform test manifest, which I'm not familiar with, so I'll leave this for bz.

Comment 23

[Liz Henry (:lizzard) (relman/hg->git project)]

Marking as a blocker. I am not sure when we will build rc3 but we will certainly wait on the rebase.

Comment 24

[Boris Zbarsky [:bzbarsky]]

> Looks like this might just be missing the rename to helper-1 here:

Indeed.  I thought I had tested all four of the files before pushing, but clearly not.  :(  Thank you for fixing this up.  I'll do the rebases.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch that applies to aurora/beta/release

Comment 26

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back this out again because of wpt failures like https://treeherder.mozilla.org/logviewer.html#?job_id=35546102&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/061471589005

Comment 27

[Boris Zbarsky [:bzbarsky]]

OK, I think I know what's going on here.  If the script in the helper-2 iframe runs before the iframe has an nsIFrame (and hence a presshell/prescontext), the click() will be ignored because nsGenericHTMLElement::CheckHandleEventForAnchorsPreconditions will return false.

I can probably replace the click test with one that does a window.open() and then a location.href set.  But it'd be good to test both codepaths...  I'll just do the click off the parent page's onload, by which point we really ought to have a prescontext in the child.

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1614e7c150b9
Set the one allowed sandbox navigator even when popups are allowed to escape the sandbox, so the new popup can be navigated by the sandboxed document.  r=smaug

Comment 29

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated version for aurora/beta/release

Comment 30

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1614e7c150b9

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2997ae20bac0

Comment 32

[Liz Henry (:lizzard) (relman/hg->git project)]

Michelle, can your team have a look at testing the new fix, now that it's landed in m-c and aurora? Thank you very much!

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/a1daf5d8cc47

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/a1daf5d8cc47

Comment 35

[Michelle Funches - QA]

QA Update: Beta, Aurora, Nightly have all been verified for the builds listed here. All is working as expected. about:blank no longer displays. GIPHY is successfully displayed.

Comment 36

[Liz Henry (:lizzard) (relman/hg->git project)]

Fantastic, thank you Michelle!

Comment 37

[Andrei Vaida [:avaida]]

As part of the 49.0-build3 sign off, we've also managed to confirm that this bug is fixed on 49. Testing covered Windows 7 x86, Windows 10 x64, Mac OS X 10.11 and Ubuntu 16.04 x64.

Updating status flags based on Comment 35 and our own work.

Comment 38

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

This has already been fixed, no need to track it now.